feat(ci): gate merges on maintainer approval via a required status check - #3176
feat(ci): gate merges on maintainer approval via a required status check#3176purp wants to merge 14 commits into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
A checkout or API failure on a merge_group run left STATUS_SHA empty, so the guard skipped and the required check sat at Expected until the queue timed out. Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Writing to $GITHUB_ENV under pull_request_target trips zizmor's github-env rule, which fails the code-scanning check. Step outputs carry the same values without granting later steps an attacker-shaped environment, and the resolved head SHA now also covers a workflow_dispatch run that fails after the pull request lookup. Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Add a required check that passes only when someone listed in MAINTAINERS.md has an approving review on the pull request. Unlike CODEOWNERS and ruleset required reviewers, a status check enforces who must approve without notifying anyone, and its approver list is a file, so outside collaborators can be listed. The job reports through its own exit code rather than a posted commit status, so it needs no write token and no head SHA. It runs on pull_request_review, and on merge_group because the queue waits for required contexts to report on the merge group ref regardless of the pull request result. MAINTAINERS.md and the decision helper are read from main, never the pull request ref, so a contributor cannot add themselves and self-approve. The helper fails closed when no handles parse. Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
b6102b0 to
f92a2df
Compare
purp
left a comment
There was a problem hiding this comment.
Refactor per comments.
Each CI tool now does one thing: check_maintainer_approval.py decides the gate, alert_maintainer_change.py renders the approver-set delta. Neither takes a verb argument. The login pattern is duplicated so each tool stands alone; a test asserts the two patterns stay identical. Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
purp
left a comment
There was a problem hiding this comment.
Please address all comments.
The change alert now prints nothing when MAINTAINERS.md moves prose without moving logins, so no comment is posted. When the updated file yields no logins at all it still posts the warning, then fails the job. The comment marker moves to a COMMENT_MARKER environment variable set in the workflow and read by both the script and the comment lookup, so the two can no longer drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
pimlock
left a comment
There was a problem hiding this comment.
This looks good, the only concern I have is about ability for the opened PR to modify the workflow that enforces the check.
I added one option to close this gap, let me know WDYT.
| on: | ||
| merge_group: | ||
| types: [checks_requested] | ||
| pull_request_review: |
There was a problem hiding this comment.
With this trigger, it's possible to overwrite the contents of the job in the PR, right?
E.g. if I change it to run: true, it would run that and report the check as successful, so I think it would be possible to create a PR, override this check, approve by anybody and merge?
The merge action is limited to anyone with write permissions though, so maybe this is fine.
There was a problem hiding this comment.
Actually, it looks like there maybe be a way to close this gap - we could add all the workflows to require codeowners approval, this way these files would be enforced by github (and consequently would ping everyone, but probably this is fine, as these changes are not as often?).
Something like
/.github/ @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr
/MAINTAINERS.md @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr
/tasks/scripts/*maintainer*.py @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr
There was a problem hiding this comment.
Here's a visualization on how it would fit in the process.
stateDiagram-v2
[*] --> AwaitingCheck: PR opened
AwaitingCheck --> Evaluating: Review submitted or dismissed
Evaluating --> Blocked: No current maintainer approval
Evaluating --> Blocked: Lookup or parsing fails
Evaluating --> Approved: Current maintainer approval exists
Blocked --> Evaluating: Review submitted or dismissed
Approved --> Evaluating: Review submitted or dismissed
Approved --> AwaitingCheck: New commit pushed
Blocked --> AwaitingCheck: New commit pushed
Approved --> AwaitingOtherRequirements
AwaitingOtherRequirements --> Evaluating: Review submitted or dismissed
AwaitingOtherRequirements --> AwaitingCheck: New commit pushed
AwaitingOtherRequirements --> MergeQueue: All requirements pass and authorized user queues PR
MergeQueue --> QueueEvaluation: merge_group event
QueueEvaluation --> QueueBlocked: Approval check or other required checks fail
QueueEvaluation --> Merged: Approval check and all other requirements pass
Merged --> [*]
note right of Evaluating
Read maintainer list and helper from main.
Fetch current reviews from GitHub.
end note
note right of AwaitingOtherRequirements
Proposed scoped CODEOWNERS protection:
changes to enforcement files require
native codeowner approval.
Ordinary PRs do not request codeowners.
end note
note right of AwaitingCheck
This PR does not trigger the check
on opening or pushing alone.
A review event starts evaluation.
end note
There was a problem hiding this comment.
I like that a lot. One moment, please ...
A workflow triggered by pull_request_review runs the pull request's copy of the workflow file rather than the one on main, so a contributor could rewrite the approval gate in the same pull request it is meant to block. Verified on this pull request: the Maintainer Approval workflow ran from the branch even though it does not exist on main. Give the enforcement paths an explicit code owner so GitHub blocks the merge natively. The tampered job would still run and still report green, but the pull request cannot merge without a code owner's approval. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
These rules named core-approval.yml and core_approval.py, which no longer exist: NVIDIA#3176 renamed the workflow to core-maintainer.yml and split the helper into check_maintainer_approval.py and alert_maintainer_change.py. CODEOWNERS matches on literal paths, so the rules protected nothing. NVIDIA#3176 carries the replacement, widened to all of .github/workflows/ and .github/actions/ rather than the single gate workflow. A workflow triggered by pull_request_review runs the pull request's own copy of the workflow file, so any workflow, not just the gate, can claim a required check context or ask for a broader token. This branch is back to its original single purpose: scoping down the wildcard rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
@mrunalp, @sjenning, and @derekwaynecarr are maintainers but not members of @NVIDIA/openshell-codeowners, and as outside collaborators they cannot join it. Owning these paths by team alone would have meant no Red Hat maintainer could approve a change to the merge gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
elezar
left a comment
There was a problem hiding this comment.
Some commits have Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> attribution.
| /tasks/scripts/check_maintainer_approval.py @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr | ||
| /tasks/scripts/alert_maintainer_change.py @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr |
There was a problem hiding this comment.
What sets these apart from other scripts? Should we add a folder for these so that they are clearly separated?
There was a problem hiding this comment.
Also, should the test files be included too?
There was a problem hiding this comment.
This script is only for "convenience", correct? The core-maintainer.yml flow SHOULD still add maintainers / CODEOWNERS to the review already. Where they can review the diff.
My question is, does adding this add any value, or does it complicate the process and increase the possible attack surface?
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| name: Maintainer Approval |
There was a problem hiding this comment.
Should the YAML file be called maintainer-approval.yml?
Summary
Adds a
OpenShell / Core Approvalcommit status that passes only when someone listed inMAINTAINERS.mdhas approved the pull request. Once registered on ruleset13332227it becomes the approval gate, which is what makes it safe to disable theCODEOWNERSwildcard in #3168.The motivating problem is that every native GitHub mechanism enforcing who must approve also notifies them:
CODEOWNERSA required status check is the only primitive that gates a merge silently. That splits enforcement from notification: this PR is the enforcement half. Targeted notification (area labels, per-area rosters) follows separately.
A file-based approver list also solves the outside-collaborator problem — three of thirteen maintainers cannot join an NVIDIA org team, but can be listed in a file.
Related Issue
No issue required. This does not touch OpenShell platform code — it is maintenance of the project's own review machinery, and is non-breaking as merged. The gate only starts enforcing when the status context is added to ruleset
13332227, which is a separate manual step (see Rollout).Changes
tasks/scripts/core_approval.py— stdlib-only helper.decideparsesMAINTAINERS.md, folds the review list to each reviewer's latest decisive position, and printsstate<TAB>description.diffrenders the approver-set delta between two versions of the file.tasks/scripts/core_approval_test.py— 17 tests.tasks/test.tomlgainstest:core-approval..github/workflows/core-approval.yml— publishes the status onpull_request_target,pull_request_review,merge_group, andworkflow_dispatch..github/workflows/maintainers-change-alert.yml— comments the added/removed handles on PRs touchingMAINTAINERS.md, so reviewers see the delta instead of diffing a markdown table..github/zizmor.yml— twodangerous-triggerssuppressions for the new workflows.Security invariants
pull_request_targetruns with a write-capable token; executing contributor code under it is the standard escalation. The checkout is pinned toref: mainwith a sparse checkout of only the helper script andpersist-credentials: false.MAINTAINERS.mdis read frommain, never the PR ref. Otherwise a contributor adds themselves in their own PR and self-approves.success.No
${{ }}interpolation appears inside anyrun:block; every context value routes throughenv:.Why this trigger set
pull_requestdoes not re-fire when someone approves, so the check would go stale red forever.pull_request_reviewfires but its jobs are not auto-surfaced as PR checks, so the status is POSTed explicitly. Fork PRs get a read-only token underpull_requestand cannot POST at all;pull_request_targetandpull_request_reviewboth run in base-repo context.merge_grouppublishes success unconditionally. A merge group only forms after the PR already satisfied this gate, and approvals cannot change while an entry sits in the queue. Without this, a required check that never reports on the merge-group ref stalls every entry for the fullcheck_response_timeout_minutes(60).Testing
mise run pre-commitpassesThere is no pre-merge dry run, by construction.
pull_request_targetruns the base branch's workflow definition, so this workflow cannot be exercised from its own PR.if: github.repository_owner == 'NVIDIA'also keeps it from running on forks. Behavior is covered by unit tests here and verified live after merge, before the check is made required.Reviewer notes
$GITHUB_ENVwrites. An earlier revision passed the head SHA between steps via$GITHUB_ENV, which trips zizmor'sgithub-envrule underpull_request_targetand fails the code-scanning check. The workflow now uses step outputs (steps.publish.outputs.posted/head_sha), which carry the same values without granting later steps an attacker-shaped environment. Note that theZizmor High reportjob is informational, but the SARIF it uploads feeds a separate Advanced Securityzizmorcheck that does fail on new alerts.workflow_dispatch, no SHA is knowable until the PR lookup runs, so a failure strictly before that point publishes nothing and logs a warning. A failure after the lookup is covered by thehead_shastep output. Every other event resolves the SHA from the event payload.dismiss_stale_reviews_on_push: true. A push and a dismissal can interleave, but the check re-runs onsynchronize, so it converges.CODEOWNERSgate. Reducing the admin surface is deliberately out of scope.johnnygrecoholds admin viaopenshell-codeownersbut is absent fromMAINTAINERS.md. Deferred; revisit when the team list is reconciled.Rollout
Order matters — reversing steps 1–3 and 4 blocks every open PR.
workflow_dispatch(pr_numberis required; there is no sweep mode). PRs opened before the workflow existed otherwise sit at "Expected" forever once the check is required.MAINTAINERS.mdcannot self-approve.OpenShell / Core Approvalto ruleset13332227.Checklist